Skip to content

Add USDT probes for Nexus background tasks #8567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bnaecker
Copy link
Collaborator

Partially closes #8424

@bnaecker bnaecker requested a review from davepacheco July 10, 2025 16:47
@@ -189,6 +192,7 @@ impl Driver {
/// If the task is currently running, it will be activated again when it
/// finishes.
pub(super) fn activate(&self, task: &TaskName) {
probes::background__task__activate!(|| task.as_str());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize the existing terminology is a little confusing here but I wonder if we should call this signal or wakeup or something? It just sounds too much like the probe that would fire when the task is activated. I suggested "signal" because I believe that's the term we use for this elsewhere ("activated by explicit signal").

I'm a little worried this call site won't cover all possible activations. I think consumers can hang onto the activator and activate directly with that?

It might also be nice to have a flag for why this was activated (timeout vs. explicit signal).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize the existing terminology is a little confusing here but I wonder if we should call this signal or wakeup or something? It just sounds too much like the probe that would fire when the task is activated. I suggested "signal" because I believe that's the term we use for this elsewhere ("activated by explicit signal").

Sounds good, I'll rename it.

I'm a little worried this call site won't cover all possible activations. I think consumers can hang onto the activator and activate directly with that?

I see that now, thanks. I can move this into the internal activation function instead.

It might also be nice to have a flag for why this was activated (timeout vs. explicit signal).

I'm kind of confused now. The background-task-activate-start probe also includes the reason. I was under the impression that the proposed background-task-signal probe was only for explicit signals, but you're proposing including the reason there as well. Could you elaborate? I must have missed the intention somewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right -- in #8424 I only mentioned explicit activation, and you're probably right that that's all we need a probe for right now. (What I was thinking when I wrote the comment above is that it'd be nice to be able to trace other types of activation, including periodic activations and activation by a dependent task. But periodic activation only ever happens from exactly one spot (with only one unique stack) so it's not that interesting. Activation by a dependency could be interesting (e.g., if you wanted to know which dependency did it and what it was doing), but I don't think we could add a useful probe for it without restructuring a lot of code. This happens when the tokio task for this background task notices that one of its dependent watch channels' values has changed. We could add a probe there, but it wouldn't have any useful context. What would be useful would be a probe when that watch channel's value actually got changed (via a send), but that point has no knowledge that it's even related to a background task. So in short, I'd ignore all of this and just do basically what you have here.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried this call site won't cover all possible activations. I think consumers can hang onto the activator and activate directly with that?

I think there's no way around this without bigger changes to the APIs. The Activator is an argument to setting up the background task, in the TaskDefinition type. The task name is not part of the Activator, since that type is entirely generic, and gets wired up in the task setup in Driver::register(). That means it's not possible to emit the probe with the name from inside the Activator type, since that type doesn't store the name.

So if we want to emit this probe when the explicit signal is made from outside the task (as opposed to the one fired when the actual implementation responds to that, which is already covered by the other probes), we either need to include the name in the Activator or remove the name from the probe entirely.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. But without the name and being in the context that actually triggered the activation, I'm not sure how useful the probe is above what we've already got with the activation-start probes. We could just leave this one out for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree it's not useful without the name. The only downside is that we don't see the request for explicit activation, just when the task starts doing its work in response to that. As long as the activating code isn't off the rails, doing something like signaling the task in a loop, that should be fine :)

@@ -0,0 +1,27 @@
#!/usr/sbin/dtrace -Zqs

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the other one, some example output would be useful here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nexus could use more DTrace probes
3 participants